feat: add completed/incompleted flag in memdisk cache, unlock storage_manager #50

lanlou1554 · 2024-04-27T23:02:38Z

The main goal is: during disk write, we don't want to hold any lock.

add completed/incompleted flag in memdisk cache
unlock disk_manager
unlock storage_manager via fine-grained locks
now our service can handle multiple same requests simultaneously without conflicts
add pin/unpin to replacer to pass the test of test_storage_manager_parallel_2, otherwise, the lock could still have concurrency issue because one thread may evict another thread's in-progress data, and after putting, get will get nothing : ( This issue could be common for mem_cache
put tests in server.rs together, otherwise it will have error thread 'server::tests::test_download_file' panicked at server.rs:217:27: error binding to 127.0.0.1:3030: error creating server listener: Address already in use (os error 48)

Solve #27, #57 partially solve #25

TODO1: should add a github issue to write data to network in put_data_to_cache, but this must change current pin/unpin strategy, since currently, the corrrectness relies on every put followed by get.

TODO2: actually in lru.rs, if the front entry is pinned, we should find the next element to evict. Current implementation only have huge problem when it is disk_cache, since the get_data will fail. But I think this scenario is rare, so I keep the current implementation for simplicity. But we need add a github issue.

~~TODO3 (important): put the unpin from get_data_from_cache from mod.rs to memory.rs & disk.rs, at the end of tokio::spawn, but it needs a lot refactor, so github issue is also needed.~~

codecov-commenter · 2024-04-27T23:05:36Z

Codecov Report

Attention: Patch coverage is 86.20690% with 88 lines in your changes are missing coverage. Please review.

Project coverage is 87.71%. Comparing base (94b59cc) to head (e7f1349).

Files	Patch %	Lines
...age-node/src/cache/data_store_cache/memdisk/mod.rs	78.26%	30 Missing and 30 partials ⚠️
storage-node/src/storage_manager.rs	90.47%	1 Missing and 17 partials ⚠️
storage-node/src/cache/replacer/lru_k.rs	93.75%	3 Missing and 1 partial ⚠️
.../cache/data_store_cache/memdisk/data_store/disk.rs	84.61%	0 Missing and 2 partials ⚠️
storage-node/src/cache/replacer/lru.rs	97.26%	2 Missing ⚠️
...ache/data_store_cache/memdisk/data_store/memory.rs	92.85%	0 Missing and 1 partial ⚠️
storage-node/src/disk/disk_manager.rs	0.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@           Coverage Diff            @@
##             main      #50    +/-   ##
========================================
  Coverage   87.71%   87.71%            
========================================
  Files          20       20            
  Lines        2539     3077   +538     
  Branches     2539     3077   +538     
========================================
+ Hits         2227     2699   +472     
- Misses        181      207    +26     
- Partials      131      171    +40

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

storage-node/src/cache/replacer/lru.rs

xx01cyx · 2024-04-28T15:02:40Z

storage-node/src/cache/replacer/lru.rs

@@ -10,7 +12,8 @@ use super::ReplacerValue;
 /// objects. The replacer will start evicting if a new object comes that makes
 /// the replacer's size exceeds its max capacity, from the oldest to the newest.
 pub struct LruReplacer<K: ReplacerKey, V: ReplacerValue> {
-    cache_map: LinkedHashMap<K, V>,
+    // usize is pin count
+    cache_map: LinkedHashMap<K, (V, usize)>,


Shall we make pin_count a field of ReplacerValue since it'll be used by all replacers?

Sure! but maybe a github issue and do it later since this pr is too large : (

storage-node/src/cache/replacer/mod.rs

storage-node/src/storage_manager.rs

storage-node/src/cache/data_store_cache/memdisk/mod.rs

xx01cyx · 2024-04-29T06:18:04Z

Maybe implement sth like this would introduce less complexity and make the code easier to write & maintain?

https://github.com/kaimast/tokio-condvar/blob/main/src/lib.rs

lanlou1554 · 2024-04-29T13:37:39Z

Maybe implement sth like this would introduce less complexity and make the code easier to write & maintain?

https://github.com/kaimast/tokio-condvar/blob/main/src/lib.rs

Cool! (I never knew a lock can be dropped : (. Updated, PTAL at the latest commit!

xx01cyx · 2024-04-29T15:35:45Z

storage-node/src/cache/data_store_cache/memdisk/mod.rs

+                            let notified = notify.0.notified();
+                            drop(status_of_keys);
+                            notified.await;


Maybe it's better to have a customized CondVar (sth. like this). But we can keep it as is and refactor it later.

we can later add a github issue to record all the refactor actions?

xx01cyx · 2024-04-29T15:36:23Z

storage-node/src/cache/data_store_cache/memdisk/mod.rs

+            let mut status_of_keys = self.status_of_keys.write().await;
+            let ((status, size), notify) = status_of_keys.get_mut(&remote_location).unwrap();
+            *status = Status::Completed;
+            *size = bytes_mem_written;
+            {
+                let mut mem_replacer = self.mem_replacer.as_ref().unwrap().lock().await;
+                mem_replacer.pin(&remote_location, *notify.1.lock().await);
+            }
+            notify.0.notify_waiters();
            return Ok(bytes_mem_written);


Shall we adopt the same logic to ensure the atomicity here as well?

let mut status_of_keys = self.status_of_keys.write().await; has already ensure the atomicity I guess lol

Also, I want to move notify.0.notify_waiters(); out of the scope of status_of_keys write lock, so waking threads can immediately access the lock?

xx01cyx · 2024-04-29T15:36:38Z

storage-node/src/cache/data_store_cache/memdisk/mod.rs

+        let ((status, size), notify) = status_of_keys.get_mut(&remote_location).unwrap();
+        *status = Status::Completed;
+        *size = data_size;
+        self.disk_replacer
+            .lock()
+            .await
+            .pin(&remote_location, *notify.1.lock().await);
+        // FIXME: disk_replacer lock should be released here
+        notify.0.notify_waiters();


And also here?

unw9527 · 2024-04-29T19:50:26Z

storage-node/src/cache/replacer/lru.rs

                debug!("-------- Evicting Key: {:?} --------", key);
-                evicted_keys.push(key);
-                self.size -= cache_value.size();
+                self.size -= cache_value.0.size();


Consider constructing another struct to replace this .0.

we can later add a github issue to record all the refactor actions : (

lanlou1554 added 3 commits April 27, 2024 17:41

Finish initial version

b6441d9

fix clippy

4e8df7e

improve details

1ea4f53

lanlou1554 requested review from xx01cyx and unw9527 April 27, 2024 23:09

lanlou1554 added 5 commits April 27, 2024 19:22

add tests in storage_manager

0ac4a69

refine test name

dafaf5e

change lock for replacer to mutex

e2c51ab

fix bugs

628990f

Resolve conflicts from main

3e09316

lanlou1554 mentioned this pull request Apr 28, 2024

refactor: reduce s3 new cost #52

Merged

feat: add pin/unpin for replacer to pass test_storage_manager_parallel_2

86e23e3

lanlou1554 marked this pull request as draft April 28, 2024 05:12

lanlou1554 mentioned this pull request Apr 28, 2024

Unpin/pin in cache replacer #25

Closed

lanlou1554 marked this pull request as ready for review April 28, 2024 05:49

fix bugs

9c19d66

lanlou1554 force-pushed the ll/unlock_disk_manager branch from 0d97e83 to 9c19d66 Compare April 28, 2024 14:33

improve pin function

5331f07

lanlou1554 mentioned this pull request Apr 28, 2024

put_data_to_cache sends data to network #56

Open

xx01cyx reviewed Apr 28, 2024

View reviewed changes

fix: unpin after all the data is consumed & fix tests for server

31015f4

xx01cyx reviewed Apr 28, 2024

View reviewed changes

lanlou1554 added 2 commits April 28, 2024 17:08

apply comment suggestions

d2b1044

apply comment suggestions

87c0d6a

lanlou1554 mentioned this pull request Apr 28, 2024

Bug of concurrency introduced by #50 #57

Closed

fix: status may change when releasing lock

7e174ec

lanlou1554 force-pushed the ll/unlock_disk_manager branch from d5d1a27 to 7e174ec Compare April 29, 2024 13:35

xx01cyx reviewed Apr 29, 2024

View reviewed changes

lanlou1554 added 5 commits April 29, 2024 12:16

improve notify_waiters

285eab8

fix: lru will find next unpin to evict

8a55d99

Merge remote-tracking branch 'origin' into ll/unlock_disk_manager

fd82bd9

modify sqlite interface

cb6a701

test: imporve evict_pinned_key test for lruk

e7f1349

unw9527 reviewed Apr 29, 2024

View reviewed changes

xx01cyx approved these changes Apr 29, 2024

View reviewed changes

lanlou1554 mentioned this pull request Apr 29, 2024

Refactor Parpulse based on #50 #59

Open

lanlou1554 merged commit c994a44 into main Apr 29, 2024
1 check passed

lanlou1554 mentioned this pull request Apr 29, 2024

Make memory and disk data store support parallel requests with conflict #33

Closed

lanlou1554 deleted the ll/unlock_disk_manager branch April 29, 2024 21:16

lanlou1554 mentioned this pull request May 2, 2024

Get_data_from_cache should also pin #76

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add completed/incompleted flag in memdisk cache, unlock storage_manager #50

feat: add completed/incompleted flag in memdisk cache, unlock storage_manager #50

lanlou1554 commented Apr 27, 2024 •

edited

Loading

codecov-commenter commented Apr 27, 2024 •

edited

Loading

xx01cyx Apr 28, 2024

lanlou1554 Apr 28, 2024

xx01cyx commented Apr 29, 2024

lanlou1554 commented Apr 29, 2024

xx01cyx Apr 29, 2024

lanlou1554 Apr 29, 2024

xx01cyx Apr 29, 2024

lanlou1554 Apr 29, 2024

lanlou1554 Apr 29, 2024

xx01cyx Apr 29, 2024

unw9527 Apr 29, 2024

lanlou1554 Apr 29, 2024

feat: add completed/incompleted flag in memdisk cache, unlock storage_manager #50

feat: add completed/incompleted flag in memdisk cache, unlock storage_manager #50

Conversation

lanlou1554 commented Apr 27, 2024 • edited Loading

codecov-commenter commented Apr 27, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xx01cyx commented Apr 29, 2024

lanlou1554 commented Apr 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lanlou1554 commented Apr 27, 2024 •

edited

Loading

codecov-commenter commented Apr 27, 2024 •

edited

Loading